Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix RSpec/NamedSubject when block has no body #1668

Merged
merged 1 commit into from
Jul 17, 2023
Merged

Fix RSpec/NamedSubject when block has no body #1668

merged 1 commit into from
Jul 17, 2023

Conversation

splattael
Copy link
Contributor

@splattael splattael commented Jul 7, 2023 •
edited
Loading

This MR fixes a regression for 👮‍♂️ RSpec/NamedSubject with EnforcedStyle: named_only in case a block has no body. For example:

describe User do
  it "is a User" do
    subject.each do
      # empty body
    end
  end
end

Exception/failure


Failures:

  1) RuboCop::Cop::RSpec::NamedSubject when EnforcedStyle is :named_only ignores subject when block has no body
     Failure/Error: block_node.body.child_nodes.find { |send_node| subject?(send_node) }

     NoMethodError:
       undefined method `child_nodes' for nil:NilClass

                 block_node.body.child_nodes.find { |send_node| subject?(send_node) }
                                ^^^^^^^^^^^^
     # ./lib/rubocop/cop/rspec/named_subject.rb:148:in `find_subject'
     # ./lib/rubocop/cop/rspec/named_subject.rb:143:in `block in nearest_subject'
     # ./lib/rubocop/cop/rspec/named_subject.rb:144:in `each'
     # ./lib/rubocop/cop/rspec/named_subject.rb:144:in `each'
     # ./lib/rubocop/cop/rspec/named_subject.rb:144:in `each'
     # ./lib/rubocop/cop/rspec/named_subject.rb:144:in `each'
     # ./lib/rubocop/cop/rspec/named_subject.rb:144:in `find'
     # ./lib/rubocop/cop/rspec/named_subject.rb:144:in `nearest_subject'
     # ./lib/rubocop/cop/rspec/named_subject.rb:134:in `subject_definition_is_named?'
     # ./lib/rubocop/cop/rspec/named_subject.rb:130:in `named_only?'
     # ./lib/rubocop/cop/rspec/named_subject.rb:121:in `allow_explicit_subject?'
     # ./lib/rubocop/cop/rspec/named_subject.rb:115:in `check_explicit_subject'
     # ./lib/rubocop/cop/rspec/named_subject.rb:103:in `block in on_block'
     # ./lib/rubocop/cop/rspec/named_subject.rb:104:in `block in subject_usage'
     # ./lib/rubocop/cop/rspec/named_subject.rb:99:in `subject_usage'
     # ./lib/rubocop/cop/rspec/named_subject.rb:102:in `on_block'
     # ./spec/support/expect_offense.rb:21:in `expect_no_offenses'
     # ./spec/rubocop/cop/rspec/named_subject_spec.rb:158:in `block (3 levels) in <top (required)>'

Finished in 0.09781 seconds (files took 0.70907 seconds to load)
20 examples, 1 failure

Failed examples:

rspec ./spec/rubocop/cop/rspec/named_subject_spec.rb:157 # RuboCop::Cop::RSpec::NamedSubject when EnforcedStyle is :named_only ignores subject when block has no body

Randomized with seed 26613

Coverage report generated for RSpec to /home/peter/devel/rubocop-rspec/coverage. 2012 / 3392 LOC (59.32%) covered.
Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected

Found at GitLab in https://gitlab.com/gitlab-org/gitlab/-/issues/249446#note_1461446904.

Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • [-] Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

@splattael
Copy link
Contributor Author

👋 @kuahyeow

Can you review this PR? 🙏

pirj
pirj reviewed Jul 8, 2023
View reviewed changes
spec/rubocop/cop/rspec/named_subject_spec.rb
it 'ignores subject when block has no body' do
expect_no_offenses(<<-RUBY)
it "is a User" do
subject.each do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a real-life example?
How did you find this in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pirj Yes, it is in the GitLab code base. See https://gitlab.com/gitlab-org/gitlab/-/issues/249446#note_1461446904

bundle exec rubocop -d --only RSpec/NamedSubject spec/lib/gitlab/github_import/importer/single_endpoint_issue_notes_importer_spec.rb

For example: https://gitlab.com/gitlab-org/gitlab/-/blob/70654c7306d007eb6bdfa970747f682a7f556718/spec/lib/gitlab/github_import/importer/single_endpoint_issue_notes_importer_spec.rb#L40

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In total, I found 38 locations where this RSpec/NamedSubject with EnforcedStyle: named_only would fail because of passing an empty block to subject's method.

spec/lib/api/helpers/authentication_spec.rb:16:6.
spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb:118:12.
spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb:130:12.
spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb:154:8.
spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb:219:4.
spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb:223:4.
spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb:231:4.
spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb:238:4.
spec/lib/gitlab/database/with_lock_retries_spec.rb:118:12.
spec/lib/gitlab/database/with_lock_retries_spec.rb:130:12.
spec/lib/gitlab/database/with_lock_retries_spec.rb:155:10.
spec/lib/gitlab/database/with_lock_retries_spec.rb:166:10.
spec/lib/gitlab/database/with_lock_retries_spec.rb:233:4.
spec/lib/gitlab/database/with_lock_retries_spec.rb:237:4.
spec/lib/gitlab/database/with_lock_retries_spec.rb:245:4.
spec/lib/gitlab/database/with_lock_retries_spec.rb:254:4.
spec/lib/gitlab/database/with_lock_retries_spec.rb:265:4.
spec/lib/gitlab/database/with_lock_retries_spec.rb:271:4.
spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb:188:6.
spec/lib/gitlab/github_import/importer/single_endpoint_diff_notes_importer_spec.rb:32:4.
spec/lib/gitlab/github_import/importer/single_endpoint_diff_notes_importer_spec.rb:51:4.
spec/lib/gitlab/github_import/importer/single_endpoint_diff_notes_importer_spec.rb:64:4.
spec/lib/gitlab/github_import/importer/single_endpoint_issue_notes_importer_spec.rb:31:4.
spec/lib/gitlab/github_import/importer/single_endpoint_issue_notes_importer_spec.rb:50:4.
spec/lib/gitlab/github_import/importer/single_endpoint_issue_notes_importer_spec.rb:63:4.
spec/lib/gitlab/github_import/importer/single_endpoint_merge_request_notes_importer_spec.rb:32:4.
spec/lib/gitlab/github_import/importer/single_endpoint_merge_request_notes_importer_spec.rb:51:4.
spec/lib/gitlab/github_import/importer/single_endpoint_merge_request_notes_importer_spec.rb:64:4.
spec/lib/gitlab/sidekiq_logging/structured_logger_spec.rb:390:6.
spec/lib/gitlab/sidekiq_middleware/size_limiter/server_spec.rb:28:2.
spec/lib/mattermost/session_spec.rb:39:6.
spec/lib/mattermost/session_spec.rb:97:8.
spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb:113:12.
spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb:127:8.
spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb:135:8.
spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb:55:8.
spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb:86:8.
spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb:98:10.

@splattael splattael force-pushed the named-subject-block-without-body branch from 674a20a to 678aceb Compare July 9, 2023 09:58
pirj
pirj approved these changes Jul 15, 2023
View reviewed changes
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

ydah
ydah approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@ydah ydah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 😊

@ydah ydah requested review from bquorning and Darhazer July 17, 2023 03:03
@pirj pirj merged commit 003e208 into rubocop:master Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ydah ydah ydah approved these changes

@pirj pirj pirj approved these changes

@bquorning bquorning Awaiting requested review from bquorning

@Darhazer Darhazer Awaiting requested review from Darhazer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@splattael @pirj @ydah